adding actuator health check support for Elasticsearch REST Clients #15211
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
|
Frankly speaking unit tests are blocked me now because I can't find a good way for writing unit tests. Can somebody give me advice? |
|
IMO, unit testing of a health indicator that interacts with a server in what can be a reasonably complex way is of limited benefit. The danger is that you end up mocking out the server in a way that doesn't match how it behaves. We've seen problems along those lines with Couchbase, for example, where the health indicator was calling something that blocked for an unacceptably long time when a node was down. If there's a Docker image available for Elasticsearch, I'd explore using Testcontainers instead. |
mbhave
added
type: enhancement
|
Thank you for your advice. It makes sense. I'll try to adding unit tests via Testcontainers. |
| @@ -263,6 +263,11 @@ | |||
| <artifactId>elasticsearch</artifactId> | |||
| <optional>true</optional> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.elasticsearch.client</groupId> | |||
| <artifactId>elasticsearch-rest-high-level-client</artifactId> | |||
There was a problem hiding this comment.
Why elasticsearch-rest-high-level-client? Spring Boot supports both elasticsearch-rest-client and elasticsearch-rest-high-level-client (which is using the former under the covers). So I think we could just have a health indicator for elasticsearch-rest-client and cover both.
There was a problem hiding this comment.
ok, I've added changes by your suggestion.
| */ | ||
|
|
||
| @Configuration | ||
| @ConditionalOnClass(RestHighLevelClient.class) |
There was a problem hiding this comment.
The RestClientAutoConfiguration is @ConditionalOnClass(RestClient.class), configuring the "low-level" RestClientBuilder in all cases, and the high level one if possible. I think we should just focus on building a low level RestClient using the auto-configured RestClientBuilder.
There was a problem hiding this comment.
I've added changes by your suggestion.
|
|
||
| @Override | ||
| protected void doHealthCheck(Health.Builder builder) throws Exception { | ||
| MainResponse info = this.client.info(RequestOptions.DEFAULT); |
There was a problem hiding this comment.
The ElasticsearchJestHealthIndicator is using the the health API, which is actually requesting the "/_cluster/health/" endpoint. I think we should align on that here. The RestHighLevelClient doesn't expose such an API, so that's another reason in favor of using the RestClient and manually issuing a request to that endpoint. Ideally, we should derive the health status from the "status" information, just like in ElasticsearchJestHealthIndicator.
There was a problem hiding this comment.
I've added changes by your suggestion.
This commit adds `ElasticsearchRestHealthIndicator`, a new `HealthIndicator` for Elasticsearch, using the Elasticsearch "low level rest client" provided by the `"org.elasticsearch.client:elasticsearch-rest-client"` dependency. Note that Spring Boot will auto-configure both low and high level REST clients, but since the high level one is using the former, a single health indicator will cover both cases. See gh-15211
this enhancement